feat(modal):customizability of chart modal #1704
Conversation
✅ Deploy Preview for carbon-charts-angular ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-charts-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-charts-core ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I have read the DCO document and I hereby sign the DCO. |
packages/core/src/model/alluvial.ts
Outdated
| const result = [ | ||
| ['Source', 'Target', 'Value'], | ||
| const headingLabels = ['Source', 'Target', 'Value'] | ||
| const tabelData = [ |
There was a problem hiding this comment.
Please correct the spelling from tabelData to tableData here and on line 22
packages/core/src/model/alluvial.ts
Outdated
| ...displayData.map((datum: any) => [datum['source'], datum['target'], datum['value']]) | ||
| ] | ||
|
|
||
| return result | ||
| return super.formatTable(headingLabels, tabelData) |
There was a problem hiding this comment.
Spelling error - tableData instead of tabelData
| get(options, 'bins.rangeLabel') || 'Range', | ||
| ...binnedStackedData.map(datum => get(datum, `0.${groupMapsTo}`)) | ||
| ] | ||
| const tabelData = [ |
There was a problem hiding this comment.
Rename tabelData to tableData
| ]) | ||
| ] | ||
|
|
||
| return result | ||
| return super.formatTable(headingLabels, tabelData) |
There was a problem hiding this comment.
Rename tabelData to tableData
packages/core/src/model/boxplot.ts
Outdated
| ['Group', 'Minimum', 'Q1', 'Median', 'Q3', 'Maximum', 'IQR', 'Outlier(s)'], | ||
| ...boxplotData.map((datum) => { | ||
| const headingLabels = ['Group', 'Minimum', 'Q1', 'Median', 'Q3', 'Maximum', 'IQR', 'Outlier(s)'] | ||
| const tabelData = [ |
There was a problem hiding this comment.
Rename tabelData to tableData
packages/core/src/model/treemap.ts
Outdated
| displayData.forEach((datum: any) => { | ||
| if (Array.isArray(datum.children)) { | ||
| datum.children.forEach((child: any) => { | ||
| result.push([child.name, datum.name, child.value]) | ||
| tabelData.push([child.name, datum.name, child.value]) |
There was a problem hiding this comment.
Rename tabelData to tableData
packages/core/src/model/treemap.ts
Outdated
| }) | ||
| } else if (getProperty(datum.name) !== null && getProperty(datum.value)) { | ||
| result.push(['–', datum.name, datum.value]) | ||
| tabelData.push(['–', datum.name, datum.value]) |
There was a problem hiding this comment.
Rename tabelData to tableData
packages/core/src/model/treemap.ts
Outdated
| } | ||
| }) | ||
|
|
||
| return result | ||
| return super.formatTable(headingLabels, tabelData) |
There was a problem hiding this comment.
Rename tabelData to tableData
packages/core/src/model/wordcloud.ts
Outdated
| const result = [ | ||
| [options.tooltip.wordLabel, 'Group', options.tooltip.valueLabel], | ||
| const headingLabels = [options.tooltip.wordLabel, 'Group', options.tooltip.valueLabel] | ||
| const tabelData = [ |
There was a problem hiding this comment.
Rename tabelData to tableData
packages/core/src/model/wordcloud.ts
Outdated
| ...displayData.map((datum: any) => [ | ||
| datum[wordMapsTo], | ||
| datum[groupMapsTo], | ||
| datum[fontSizeMapsTo] | ||
| ]) | ||
| ] | ||
|
|
||
| return result | ||
| return super.formatTable(headingLabels, tabelData) |
There was a problem hiding this comment.
Rename tabelData to tableData
packages/core/src/model/model.ts
Outdated
| @@ -46,6 +41,34 @@ export class ChartModel { | |||
| this.services = services | |||
| } | |||
|
|
|||
| formatTable(headingLabels, tableData) { | |||
There was a problem hiding this comment.
can we pls update this function to just take an object instead of 2 parameters
e.g. formatTable({ headers: [...], cells: [...] }) this way it's easier to read
packages/core/src/model/model.ts
Outdated
| @@ -46,6 +41,34 @@ export class ChartModel { | |||
| this.services = services | |||
| } | |||
|
|
|||
| formatTable(headingLabels, tableData) { | |||
| const options = this.getOptions() | |||
| const headingFormatter = getProperty(options, 'modal', 'headingFormatter') | |||
There was a problem hiding this comment.
let's call this options.tabularRepresentation.headingFormatter, modal seems a bit too generic
There was a problem hiding this comment.
let's call this tableHeadingFormatter
packages/core/src/model/model.ts
Outdated
| formatTable(headingLabels, tableData) { | ||
| const options = this.getOptions() | ||
| const headingFormatter = getProperty(options, 'modal', 'headingFormatter') | ||
| const valueFormatter = getProperty(options, 'modal', 'valueFormatter') |
There was a problem hiding this comment.
let's call this tableCellFormatter
packages/core/src/model/model.ts
Outdated
| const options = this.getOptions() | ||
| const headingFormatter = getProperty(options, 'modal', 'headingFormatter') | ||
| const valueFormatter = getProperty(options, 'modal', 'valueFormatter') | ||
| const tableFormatter = getProperty(options, 'modal', 'tableFormatter') |
There was a problem hiding this comment.
tableFormatter is a great name for this one
packages/core/src/model/model.ts
Outdated
| domainValueFormatter = (d: any) => format(d, 'MMM d, yyyy') | ||
| } | ||
| const result = [ | ||
| headingFormatter && typeof headingFormatter === 'function' |
There was a problem hiding this comment.
| headingFormatter && typeof headingFormatter === 'function' | |
| typeof headingFormatter === 'function' |
There was a problem hiding this comment.
wouldn't just checking the type be enough?
packages/core/src/model/model.ts
Outdated
| @@ -46,6 +41,30 @@ export class ChartModel { | |||
| this.services = services | |||
| } | |||
|
|
|||
| formatTable({ headers, cells }) { | |||
| const options = this.getOptions() | |||
| const tableHeadingFormatter = getProperty(options, 'modal', 'tableHeadingFormatter') | |||
There was a problem hiding this comment.
| const tableHeadingFormatter = getProperty(options, 'modal', 'tableHeadingFormatter') | |
| const tableHeadingFormatter = getProperty(options, 'tabularRepModal', 'tableHeadingFormatter') |
packages/core/src/model/model.ts
Outdated
| formatTable({ headers, cells }) { | ||
| const options = this.getOptions() | ||
| const tableHeadingFormatter = getProperty(options, 'modal', 'tableHeadingFormatter') | ||
| const tableCellFormatter = getProperty(options, 'modal', 'tableCellFormatter') |
There was a problem hiding this comment.
| const tableCellFormatter = getProperty(options, 'modal', 'tableCellFormatter') | |
| const tableCellFormatter = getProperty(options, 'tabularRepModal', 'tableCellFormatter') |
packages/core/src/model/model.ts
Outdated
| const options = this.getOptions() | ||
| const tableHeadingFormatter = getProperty(options, 'modal', 'tableHeadingFormatter') | ||
| const tableCellFormatter = getProperty(options, 'modal', 'tableCellFormatter') | ||
| const tableFormatter = getProperty(options, 'modal', 'tableFormatter') |
There was a problem hiding this comment.
| const tableFormatter = getProperty(options, 'modal', 'tableFormatter') | |
| const tableFormatter = getProperty(options, 'tabularRepModal', 'tableFormatter') |
Updates
Demo screenshot or recording